I think it makes sense to rather do two loops. One over mainRepoIDs and instead have another list for submoduleIDs. The actual code is quite minimal for submoduleIDs and the duplication then between what we do once we have created a doc is minimal.

Done — split into two separate loops. mainRepoKeys iterated with the streaming reader, submoduleKeys iterated with go-git createDocument. The builder sorts documents internally via sortDocuments so ordering between the two loops does not matter.

I think there is a problem with this current API in that you read everything into memory. This just won't scale for very large repos. It's part of why zoekt works with this design of adding documents that at certain thresholds decide to create a shard (which then rely on the GC to free memory).

Additionally previously if a blob was over our max size, we just wouldn't read it in to memory.

I would suggest instead designing the API to be like the archive/tar API. So you would still pass in a huge slice of mainRepoIDs. But it would then return a stateful struct over the output from the process. For each file we get a little bit of metadata (like size, etc) and then the caller here can read that into a []byte or call .Next to get it to skip over the file.

So I think to keep the nice property around just passing in all IDs, we still get git to output all objects. But here in go land we decide between actually reading the object into a []byte or just io.Discarding it.

You don't have to go crazy on the design of the API. Just do something nice and minimal to suit this use case.

Agreed, reworked. readBlobsPipelined is replaced by a streaming catfileReader — archive/tar.Reader-style:

cr, _ := newCatfileReader(opts.RepoDir, mainRepoIDs)
for _, key := range mainRepoKeys {
    size, missing, _ := cr.Next()
    if size > SizeMax { continue } // unread bytes auto-discarded by next Next()
    content := make([]byte, size)
    io.ReadFull(cr, content)
}

• Next() reads the header and returns size — content is not read until the caller calls Read
• Calling Next() again without reading discards unread bytes via bufio.Reader.Discard
• Large blobs never touch Go memory — only the header is parsed, content is discarded in the bufio buffer
• Peak memory is now bounded by ShardMax (one shard's worth of content), not total repo size

also defer close(writeErr) so if you call Close twice it won't block forever.

Done — added defer close(writeErr) in the goroutine. Also wrapped Close() itself in sync.Once (same pattern as DirectoryWatcher.Stop in search/watcher.go) so that cmd.Wait() is also called exactly once. This makes Close fully idempotent — safe to call multiple times or concurrently. Added TestCatfileReader_DoubleClose and TestCatfileReader_ConcurrentClose that verify this.

I worry about what happens when we call Close before git is finished giving us all its output. eg if we get an error during builder.Add we will call close before reading everything.

Maybe instead this should just always call something like cr.cmd.Kill() (I can't remember offhand what it is). Then I think all the intermediatte goroutines/etc will drain.

I do quite like that you nicely clean up here, but I want to make sure we have good behaviour for early termination.

Good call — switched to kill-first. Close() now calls cmd.Process.Kill() before draining. This matches Gitaly's pattern — their cache eviction code has an explicit comment: "We first cancel the context such that the process gets a SIGKILL signal. Calling close() first may lead to a deadlock given that it waits for the process to exit, which may not happen if it hangs writing data to stdout."

After Kill, the drain completes instantly (stdout pipe gets EOF), the writer goroutine gets EPIPE on its next write and exits, and cmd.Wait() returns. The expected "signal: killed" exit error is suppressed via isKilledErr().

Added TestCatfileReader_CloseBeforeExhausted_ManyBlobs (200 files, reads only 1, then Close) and TestCatfileReader_CloseWithoutReading to exercise this.

minor: lets parse this as an int instead of int64 since all call sites work with int and then you can avoid the cast to int when you do Discard

Done — Next() now returns int, pending field is int, uses strconv.Atoi. Removed all casts at call sites (Discard, size comparison against SizeMax, make([]byte, size)).

have you come across missing in your tests? Saying we skipped it due to large file doesn't seem correct. We should either fail or if this has a common reason add a new indexing reason for missing a document.

You're right — added SkipReasonMissing with explanation "object missing from repository". Missing blobs now get skippedDoc(key, branches, index.SkipReasonMissing) plus a warning log. This is unexpected for local repos (could indicate corruption, shallow clone, or a race with git gc), so the warning helps operators diagnose it.

Updated file_category.go to handle SkipReasonMissing the same way as TooLarge — guess category from filename, don't examine content.

The go-git createDocument path keeps using SkipReasonTooLarge for ErrObjectNotFound since that has a different semantic (large objects excluded during fetch).

minor: unusual to repeatadly call io.ReadFull on the same reader. You might be able to expose a nicer api with the use of io.LimitedReader. But this is a very minor point, this does work!

Considered this — io.ReadFull into a pre-allocated buffer is actually more efficient here since the size is known: zero intermediate allocations vs io.ReadAll(io.LimitReader(...)) which grows a buffer internally. Added a comment explaining the choice. The catfileReader already limits reads internally via the pending field (same approach as archive/tar.Reader), so an extra LimitedReader wrapper would be redundant.

What I meant is the catfileReader API exposes a LimitedReader rather than directly using it's Read implementation. But yeah, this all works so was more just me thinking it through. For example I think that is how the archive reader API likely works.

Feels a bit error prone the number of places we need to remember to do cr.Close inside of this if statement. Maybe this whole if statement body can be factored out into a function, then we can do defer cr.Close?

Done — extracted indexCatfileBlobs() with defer cr.Close(). This eliminated all four manual cr.Close() calls (three error paths + one happy path). Any new error path added in the future is automatically covered by the defer.

I have a slight concern this breaks stuff. For a few weeks can we have an envvar switch off this behaviour (I guess if turned off then if this if statement is always false it will just work out?)

Then after a few weeks we remove the envvar. But this code path is pretty critical on the server side both at Sourcegraph and GitLab.

Done — added ZOEKT_DISABLE_CATFILE_BATCH. Follows the exact same pattern as ZOEKT_DISABLE_GOGIT_OPTIMIZATION already in this function: cmp.Or(os.Getenv(...), "false") + strconv.ParseBool. When set to true, the if key.SubRepoPath == "" condition is effectively always false, so all keys go through the go-git createDocument path. Benchmarked both paths on kubernetes — cat-file path is 4.4x faster, go-git fallback works correctly.

What I meant is the catfileReader API exposes a LimitedReader rather than directly using it's Read implementation. But yeah, this all works so was more just me thinking it through. For example I think that is how the archive reader API likely works.